Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cubic splines overhaul #10701

Merged
merged 90 commits into from
Feb 28, 2024
Merged

Cubic splines overhaul #10701

merged 90 commits into from
Feb 28, 2024

Conversation

JohnTheCoolingFan
Copy link
Contributor

@JohnTheCoolingFan JohnTheCoolingFan commented Nov 22, 2023 •
edited
Loading

Objective

Improve the bevy::math::cubic_splines module by making it more flexible and adding new curve types.
Closes #10220

Solution

Added new spline types and improved existing

Changelog

Added

  • CubicNurbs rational cubic curve generator, allows setting the knot vector and weights associated with every point
  • LinearSpline curve generator, allows generating a linearly interpolated curve segment
  • Ability to push additional cubic segments to CubicCurve
  • IntoIterator and Extend implementations for CubicCurve

Changed

  • Point trait has been implemented for more types: Quat and Vec4.
  • CubicCurve::coefficients was moved to CubicSegment::coefficients because the function returns CubicSegment, so it seems logical to be associated with CubicSegment instead. The method is still not public.

Fixed

  • CubicBSpline::new was referencing Cardinal spline instead of B-Spline

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Math Fundamental domain-agnostic mathematical operations labels Nov 22, 2023
@JohnTheCoolingFan JohnTheCoolingFan changed the title Added Nurbs spline Cubic splines overhaul Nov 22, 2023
@JohnTheCoolingFan
Helper methods for generating various knot vectors
344c942
@JohnTheCoolingFan
Added IntoIterator and Extend implementations for CubicCurve
70a13c5
@JohnTheCoolingFan
Added push_segment method
19e6772
@JohnTheCoolingFan
Added linear spline
004a72c
@JohnTheCoolingFan
Make Point trait impl a blanket impl
ae63196
@JohnTheCoolingFan
Removed unused imports
22542c1
@JohnTheCoolingFan
Moved some math to helper associated functions
7972a72
@JohnTheCoolingFan
Fixed open uniform knot vector generation function
6084556
@JohnTheCoolingFan
Implementation for basis matrix generation
d738eed
@JohnTheCoolingFan
Implemented cubci generator for nurbs
6246848
@JohnTheCoolingFan
Weight normalization and application
dec3be5
@JohnTheCoolingFan
Fixed lint error
95c8e08
@JohnTheCoolingFan
Moved coefficients computation to CubicSegment
0101867
@JohnTheCoolingFan
Fixed typo in comment
c1a5105
@JohnTheCoolingFan
Extract multiplied index to a variable
1407647
@JohnTheCoolingFan
Remoevd panics with Result and error enum
2e0a2a4
@JohnTheCoolingFan
Improvbed wording in open uniform knot vector fn documentation
42de336
@JohnTheCoolingFan
Removed code that wasn't doing anything
e3f45e5
@JohnTheCoolingFan
Made weight normalization use L0 norm
754d3fe
@JohnTheCoolingFan
Copy link
Contributor Author

The only thing left is documentation for the new types and methods, I think

@JohnTheCoolingFan
Copy link
Contributor Author

Ok, the Point trait is no more a blanket implementation and more types implement it, specifically Vec4 and Quat. There are reasons why I didn't implement it on more types:

  1. glam's MatX do not implement Div<f32>, only Mut<f32>, which I reported here: impl Div<f32>/Div<f64> for [D]MatX bitshifter/glam-rs#476
  2. Color from bevy_color and other color types also do not implement all the math operation traits required. I'm not sure whether implementing them wouldn't produce anything undesirable, so I would like to get more opinions, and implementing this would probably be better as a different PR
  3. f64 and types based on that don't interoperate with f32. So unless Point gets a generic argument or associated type specifying the type (f32, f64, potentially even more if it makes sense). Same for integers.

@alice-i-cecile
Copy link
Member

Like I said on Discord, I'd like to leave the Color implementations for follow-up :) Doing a final review pass now.

@alice-i-cecile alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Feb 28, 2024
alice-i-cecile
alice-i-cecile approved these changes Feb 28, 2024
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with this now. Excellent work, and thanks for your patience. I'd like to see more learning materials / examples for this area of the engine, but that can definitely be done in follow-up.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 28, 2024
Jondolf
Jondolf reviewed Feb 28, 2024
View reviewed changes
crates/bevy_math/src/cubic_splines.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/cubic_splines.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/cubic_splines.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/cubic_splines.rs Outdated Show resolved Hide resolved
@JohnTheCoolingFan @Jondolf
Typos and wording fixes
32c35fc 
Co-authored-by: Joona Aalto <[email protected]>
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 28, 2024
Merged via the queue into bevyengine:main with commit a543536 Feb 28, 2024
27 of 29 checks passed
This was referenced May 30, 2024
Closed
Closed
JohnTheCoolingFan added a commit to JohnTheCoolingFan/bevy-website that referenced this pull request Jun 4, 2024
@JohnTheCoolingFan
bevyengine/bevy#10701 release notes
72cd46a 
Initial release notes for the Cubic Splines overhaul PR.
@JohnTheCoolingFan JohnTheCoolingFan mentioned this pull request Jun 4, 2024
Merged
JohnTheCoolingFan added a commit to JohnTheCoolingFan/bevy-website that referenced this pull request Jun 4, 2024
@JohnTheCoolingFan
bevyengine/bevy#10701 release notes
35848f4 
Initial release notes for the Cubic Splines overhaul PR.
@alice-i-cecile alice-i-cecile removed the M-Needs-Release-Note Work that should be called out in the blog due to impact label Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jondolf Jondolf Jondolf left review comments

@IQuick143 IQuick143 IQuick143 left review comments

@NthTensor NthTensor NthTensor approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@aevyrie aevyrie aevyrie left review comments

@RobWalt RobWalt RobWalt approved these changes

Assignees
No one assigned
Labels
A-Math Fundamental domain-agnostic mathematical operations C-Feature A new feature, making something new possible D-Complex Quite challenging from either a design or technical perspective. Ask for help! S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
0.14
Development

Successfully merging this pull request may close these issues.

NURBS spline for generating CubicCurve
7 participants
@JohnTheCoolingFan @IQuick143 @NthTensor @alice-i-cecile @aevyrie @RobWalt @Jondolf